Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add a way to open the admin settings overview directly #33756

Merged
merged 3 commits into from
Aug 31, 2022
Merged

add a way to open the admin settings overview directly #33756

merged 3 commits into from
Aug 31, 2022

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Aug 30, 2022 •
edited
Loading

This is best reviewed like this: https://github.com/nextcloud/server/pull/33756/files?diff=unified&w=1

Motivation

I think this is a good idea and the community seems to agree: https://redd.it/x11gue

Also Jan said that it is fine by him :)

Screenshot

click here

image

Signed-off-by: szaimen szaimen@e.mail.de

For my own testing 
docker run -it \
-e SERVER_BRANCH=enh/noid/admin-settings-directly \
-p 8443:443 \
-e TRUSTED_DOMAIN=192.168.146.130 \
--name nextcloud-easy-test \
ghcr.io/szaimen/nextcloud-easy-test:latest

@szaimen
add a way to open the admin settings overview directly
955b6bb 
Signed-off-by: szaimen <szaimen@e.mail.de>
@szaimen szaimen added enhancement 3. to review Waiting for reviews labels Aug 30, 2022
@szaimen szaimen added this to the Nextcloud 25 milestone Aug 30, 2022
jancborchardt
jancborchardt approved these changes Aug 30, 2022
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! :)

We should also replace the icons with Material Design icons, but separate issue probably.

Then we can also use separate icons for the settings:

  • Personal: person
  • Admin: settings

@come-nc
Copy link
Contributor

come-nc commented Aug 30, 2022 •
edited
Loading

But then it should be two separated pages, with each its own sidebar menu?

If I understand the current PR both menu entry lead to the same page, just with a different default "tab"?

@szaimen
Copy link
Contributor Author

szaimen commented Aug 30, 2022

If I understand the current PR both menu entry lead to the same page, just with a different default "tab"?

Yes but that already makes a huge positive difference in my personal testing!

@szaimen
Copy link
Contributor Author

szaimen commented Aug 30, 2022

Looks good! :)

We should also replace the icons with Material Design icons, but separate issue probably.

Yes separate issuey however I could replace the personal settings icon with https://github.com/nextcloud/server/blob/master/apps/settings/img/personal.svg. WDYT?

@blizzz blizzz mentioned this pull request Aug 30, 2022
Merged
@szaimen szaimen force-pushed the enh/noid/admin-settings-directly branch 2 times, most recently from b9513a9 to b2885aa Compare August 30, 2022 20:06
@szaimen szaimen requested review from danxuliu and Pytal August 30, 2022 21:34
@danxuliu
Copy link
Member

I visit the admin settings page should open the admin settings, not the personal settings of the admin, so I added a fixup commit for that; if the personal settings of the admin need to be opened a new I visit the personal settings page step should be added, but it is not currently needed, as all* the acceptance tests that open the admin settings do that to access an admin-level settings section.

*Except the one in access-levels test, which just needs to open the settings page to check the available panels.

@szaimen
Copy link
Contributor Author

szaimen commented Aug 31, 2022 •
edited
Loading

I visit the admin settings page should open the admin settings, not the personal settings of the admin, so I added a fixup commit for that; if the personal settings of the admin need to be opened a new I visit the personal settings page step should be added, but it is not currently needed, as all* the acceptance tests that open the admin settings do that to access an admin-level settings section.

*Except the one in access-levels test, which just needs to open the settings page to check the available panels.

fine by me as long as the tests pass :)

BTW, is /drone/src/tests/acceptance/features/users.feature:48 a flaky test?

And can you please squash or shall I do it?

@szaimen
fix tests
17ee17c 
Signed-off-by: szaimen <szaimen@e.mail.de>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@szaimen szaimen force-pushed the enh/noid/admin-settings-directly branch from db221b0 to 17ee17c Compare August 31, 2022 12:35
@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 31, 2022
@szaimen
Copy link
Contributor Author

szaimen commented Aug 31, 2022

And can you please squash or shall I do it?

done

@jancborchardt
Copy link
Member

Looks good! :)

We should also replace the icons with Material Design icons, but separate issue probably.

Yes separate issuey however I could replace the personal settings icon with https://github.com/nextcloud/server/blob/master/apps/settings/img/personal.svg. WDYT?

@szaimen yes, that would be a good fix! :)

@szaimen
add a better icon for personal settings
0c90474 
Signed-off-by: szaimen <szaimen@e.mail.de>
@szaimen
Copy link
Contributor Author

szaimen commented Aug 31, 2022

@szaimen yes, that would be a good fix! :)

done :)

@danxuliu
Copy link
Member

BTW, is /drone/src/tests/acceptance/features/users.feature:48 a flaky test?

It seems so. Unfortunately I do not see why, and I locally run it 50 times and all of them passed, so... something for later ;-)

And can you please squash or shall I do it?

Done

Thanks :-)

@szaimen
Copy link
Contributor Author

szaimen commented Aug 31, 2022

CI failure unrelated

@szaimen szaimen merged commit b0f6ca0 into master Aug 31, 2022
@szaimen szaimen deleted the enh/noid/admin-settings-directly branch August 31, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@artonge artonge Awaiting requested review from artonge

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

@come-nc come-nc Awaiting requested review from come-nc

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@danxuliu danxuliu Awaiting requested review from danxuliu

@Pytal Pytal Awaiting requested review from Pytal

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
5 participants
@szaimen @come-nc @danxuliu @jancborchardt @CarlSchwan